Don't run cleanups twice in "if true" blocks #10735
Conversation
Turns out `with_scope` already translates destructors, so by manually translating destructors we end up running them all twice (bad). Closes rust-lang#10734
|
|
||
| pub fn main() { | ||
| if true { | ||
| let _a = ~3; |
There was a problem hiding this comment.
It seems not quite ideal to rely on throwing a pointer being freed was not allocated malloc error to indicate the bug exists. I would suggest something like the following:
#[unsafe_no_drop_flag]
struct Foo {
dropped: bool
}
impl Drop for Foo {
fn drop(&mut self) {
assert!(!self.dropped);
self.dropped = true;
}
}
pub fn main() {
if true {
let _a = Foo{ dropped: false };
}
if false {
fail!();
} else {
let _a = Foo{ dropped: false };
}
}
There was a problem hiding this comment.
After chatting with @cmr, it might actually be nice to assert that it's dropping at all, just to kill two birds with one stone (i.e. define this test as checking that it drops the correct number of times, rather than checking that it hasn't dropped too many times).
static mut drop_count: uint = 0;
#[unsafe_no_drop_flag]
struct Foo {
dropped: bool
}
impl Drop for Foo {
fn drop(&mut self) {
assert!(!self.dropped);
self.dropped = true;
unsafe { drop_count += 1; }
}
}
pub fn main() {
if true {
let _a = Foo{ dropped: false };
}
unsafe { assert!(drop_count == 1); }
if false {
fail!();
} else {
let _a = Foo{ dropped: false };
}
unsafe { assert!(drop_count == 2); }
}
There was a problem hiding this comment.
Although, come to think about it, this is not quite right as it's making a (probably bad) assumption about where the drops occur. It should probably be something like this instead:
pub fn main() {
{
if true {
let _a = Foo{ dropped: false };
}
if false {
fail!();
} else {
let _a = Foo{ dropped: false };
}
}
unsafe { assert!(drop_count == 2); }
}
There was a problem hiding this comment.
I feel like I'm having a conversation with myself.
After thinking some more, the previous version (with the two asserts) isn't so bad, because that's also checking to make sure that if true { expr } behaves like { expr } rather than behaving like expr, which seems to me to be the correct interpretation. FWIW, that version (again, the one with the two asserts) passes with this patch applied.
Turns out `with_scope` already translates destructors, so by manually translating destructors we end up running them all twice (bad). Closes #10734
Bump README copyright changelog: none
Turns out
with_scopealready translates destructors, so by manuallytranslating destructors we end up running them all twice (bad).
Closes #10734